fix(runtime): guard Windows bundled runtime arch detection#94
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the runtime architecture detection logic on Windows. It prevents errors when the system encounters an unsupported host architecture by implementing guard clauses, while ensuring that explicit architecture overrides remain fully functional. This change improves the stability and predictability of the application's runtime environment setup. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
hasBundledRuntimeOverride/hasTargetArchOverridevariables currently hold string values; consider explicitly coercing them to booleans (e.g.Boolean(String(...).trim())) so their types better match the naming and avoid accidental truthiness issues if the logic is reused. - The override-detection logic added in
isWindowsArm64BundledRuntimeis somewhat verbose; consider extracting a small helper (e.g.hasRuntimeArchOverride(env)) to keep this function focused on its main decision and make future changes to the override rules easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `hasBundledRuntimeOverride` / `hasTargetArchOverride` variables currently hold string values; consider explicitly coercing them to booleans (e.g. `Boolean(String(...).trim())`) so their types better match the naming and avoid accidental truthiness issues if the logic is reused.
- The override-detection logic added in `isWindowsArm64BundledRuntime` is somewhat verbose; consider extracting a small helper (e.g. `hasRuntimeArchOverride(env)`) to keep this function focused on its main decision and make future changes to the override rules easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a potential crash in isWindowsArm64BundledRuntime when running on an unsupported host architecture without explicit overrides. The fix correctly introduces a guard to return false in this scenario, and the new regression test properly covers this case. I have one suggestion to improve the readability of the new logic by reducing code duplication.
| const hasBundledRuntimeOverride = | ||
| env[BUNDLED_RUNTIME_ARCH_ENV] !== undefined && String(env[BUNDLED_RUNTIME_ARCH_ENV]).trim(); | ||
| const hasTargetArchOverride = | ||
| env[DESKTOP_TARGET_ARCH_ENV] !== undefined && String(env[DESKTOP_TARGET_ARCH_ENV]).trim(); |
There was a problem hiding this comment.
To improve readability and reduce code duplication, you could extract the repeated logic for checking if an environment variable is set to a non-empty string into a small helper function within the scope of this function.
| const hasBundledRuntimeOverride = | |
| env[BUNDLED_RUNTIME_ARCH_ENV] !== undefined && String(env[BUNDLED_RUNTIME_ARCH_ENV]).trim(); | |
| const hasTargetArchOverride = | |
| env[DESKTOP_TARGET_ARCH_ENV] !== undefined && String(env[DESKTOP_TARGET_ARCH_ENV]).trim(); | |
| const isSet = (v) => v !== undefined && String(v).trim(); | |
| const hasBundledRuntimeOverride = isSet(env[BUNDLED_RUNTIME_ARCH_ENV]); | |
| const hasTargetArchOverride = isSet(env[DESKTOP_TARGET_ARCH_ENV]); |
|
@sourcery-ai review |
1 participant
Summary
isWindowsArm64BundledRuntimereturnfalseinstead of throwing when it runs onwin32with an unsupported hostprocess.archand no explicit arch overrideTest Plan
node --test scripts/backend/runtime-arch-utils.test.mjspnpm run test:prepare-resourcesSummary by Sourcery
Guard Windows bundled runtime architecture detection against unsupported host architectures and centralize runtime-arch override handling.
Bug Fixes:
Enhancements:
Tests: